Txpool 4844 upgrades Part 2 #1125
Conversation
somnathb1
changed the title
somnathb1
changed the title
txpool/pool.go
Outdated
| func (p *TxPool) setBlobFee(blobFee uint64) (uint64, bool) { | ||
| changed := false | ||
| if blobFee > 0 { | ||
| changed = blobFee != p.pendingBlobFee.Load() |
There was a problem hiding this comment.
“Read then write” on atomic - it’s race-condition. Please use CompareAndSwap operation.
There was a problem hiding this comment.
That's the only place of writing that variable tho, but there is a bit of unnecessary code here. Removing it.
txpool/pool.go
Outdated
| if _, ok := p.minedBlobTxsByHash[string(mt.Tx.IDHash[:])]; ok { | ||
| p.deleteMinedBlobTxn(string(mt.Tx.IDHash[:])) | ||
| // Don't add blob tx to queued if it's less than current pending blob base fee | ||
| if mt.Tx.Type == types.BlobTxType && mt.Tx.BlobFeeCap.Cmp(uint256.NewInt(p.pendingBlobFee.Load())) < 0 { |
There was a problem hiding this comment.
uint256 has method Less
There was a problem hiding this comment.
Changed it to native uint64 comparison
There was a problem hiding this comment.
Switched to using uint256's CmpUint64() and LtUint64() methods
txpool/pool.go
Outdated
| logger log.Logger) { | ||
| // Demote worst transactions that do not qualify for pending sub pool anymore, to other sub pools, or discard | ||
| for worst := pending.Worst(); pending.Len() > 0 && (worst.subPool < BaseFeePoolBits || worst.minFeeCap.Cmp(uint256.NewInt(pendingBaseFee)) < 0); worst = pending.Worst() { | ||
| for worst := pending.Worst(); pending.Len() > 0 && (worst.subPool < BaseFeePoolBits || worst.minFeeCap.Cmp(uint256.NewInt(pendingBaseFee)) < 0 || (worst.Tx.Type == types.BlobTxType && worst.Tx.BlobFeeCap.Cmp(uint256.NewInt(pendingBlobFee)) < 0)); worst = pending.Worst() { |
There was a problem hiding this comment.
Can we avoid “NewInt” inside loop - for optimization. Because we are also inside global mutex of txpool - and better unlock it as fast as possible.
There was a problem hiding this comment.
Changed it to native uint64 conversion and then comparison
There was a problem hiding this comment.
Is it in “wei”? Can it have uint64 overflow?
There was a problem hiding this comment.
Eventually comes from the submitted transaction, theoretically it could have an overflow. So could the block's baseFee. I will make the worst assumptions here and rather use uint256 everywhere.
Practically, it may not matter, but for expanding support to other chains later, with much lower native token values, it could be useful.
There was a problem hiding this comment.
Switched to using uint256's CmpUint64() and LtUint64() methods
somnathb1
dismissed
AskAlexSharov’s stale review
Regarding fuzz tests: Existing ones work. For changes specific to this PR, unit tests and Hive tests would be used to test these. I could introduce few changes/additions to txpool fuzz tests targeted at the changes for 4844
Some peer-review changes from the last related PR. Addition of a flag for BlobSlots - for max allowed blobs per account in txpool. Use BlobFee from the block to validate txs in the pool. See also ledgerwatch/erigon-lib#1125
Fix some peer-review comments from the last related PR, and add some enhancements
Change summary
Related: erigontech/erigon#8213 erigontech/interfaces#195